-
Notifications
You must be signed in to change notification settings - Fork 831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MusicKeyboard: BugFixes, ES6 port, linting. #2796
Conversation
ed904a4
to
29cb58f
Compare
Merging default branch into feature branches is not a good practice. If it is essential, perform a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Text on keys are to be visible only for pitches inside the widget's block.
- Add a margin/padding at the bottom of the text on the black keys.
- Any modification action on the column labels (duration) at the bottom of the matrix must close the pie menu.
- Any modifications on the pitch labels on the left of the matrix should reflect immediately in the widget's blocks. That includes adding new pitch/hertz blocks.
- The matrix seems to behave oddly after changing a pitch, closing the widget, and reopening it. Check pls.
I performed an |
Didn't get you this time. Can you please come again?
I checked for both of these behaviors with My current local and master(musicblocks website), they are the same. Please tell me if I am missing something here. Thanks |
I know that. Somehow regressions have been introduced. This isn't meant to behave in this fashion. Gotta fix them. |
Well then, perhaps could use different colors. Something lighter for all placeholder labels and and ones whose blocks are present, highlighted. Although, the idea is that only the pitches present in the widget's clamp would be required to play the music the user wants. |
any suggestions on which color i should go ahead with? |
Try to do the rest first. Perhaps we can discuss this after. |
Sure About the modifications that you have referenced, They work smoothly for me, have a look at the test-video below for your perusal musickeyboard.mp4Notice at the bottom right of how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add pitch/hertz doesn't work as expected. When you add a note, force a reload.
New created blocks are being inserted into the widget clamp block when you add another. e.g. you try to add a hertz block, it doesn't show. You then add a pitch, the pitch isn't inserted, but the previous hertz block now gets added.
Right, This is a regression and it's prevalent in all widgets where in we can add new notes be it pitch, hertz or drum. I tried testing it with Phrasemaker widget and behavior was erratic there as well. I am not quite sure where this bug is, will dig and try to solve it in a different PR. As is, this PR focused on porting, Linting and bug that I have described above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a contrasting shade of grey from platformColor
for the ones not in the block.
Um, wouldn't that scuttle the UI of Keyboard? IRL, white and black-keys is how most or probably every music keyboard looks like. Do we really want mix of colors: ones that are in and that are not in "block"? |
@walterbender what do you think about labels for pitches not inside the widget clamp? Right now, those aren't labeled. |
Regarding labelling, I think we should only label the keys designated by the user in the widget clamp. The other keys are there to provide context/structure. As far as the labels themselves, where possible, they should be letter names since the keys of a piano are unambiguous. But a hertz block may or may not map onto a letter name, depending on the temperament, so I think that hertz should be a number and treated separately as per the current design. |
I would rather have the "rest" be under the keyboard, like a space key on a typewriter, but that is outside of the scope of this PR. |
@walterbender so any issues with this PR ?? |
ps:- Firstly, I was planning on working on the bug(regression) @meganindya has rightly pointed out when we add new notes/pitches/drums (in phrasemaker widget). I am working on finding the cause of the regression, I think I'm close, hopefully will raise a PR addressing it soon. Then, eventually foray into working on the UI enhancements as suggested above. |
I will try to review this weekend. |
Apologies if I have misinterpreted this all along, when I started working on this widget, I was under the impression that Black Keys not displaying the text that they're supposed to(irrespective of whether or not they're part of blocks we've added{clamps}) , is a bug ;-; Any suggestions on changes I should make? |
There had been a bug where the black keys were being mislabeled. But I thought that was fixed. |
It is really impossible to review this since there are so many unrelated changes scattered throughout your commits. It is good practice to confine each commit to one theme: (1) adding doc strings; (2) lint formatting changes; (3) bug fix. As it is I don't know where to look for your bug fix. |
Could you please submit a new PR with three separate commits, one each for your bug fix, your ES6 port, and your linting. |
Issue references:- #2767, #2630, #2609 #2629
In this PR, I have:-
widgets/musickeyboard.js
file to ES6 class syntax.Bug
MusicKeyboard's black keys don't display text ( i.e. which key they represent respectively)
Obviation of bug post fixes and also the test after ES6 PORT.
MusicKeyboard_After.mp4